Add fido entry point to davinci client package - #744
Conversation
|
|
View your CI Pipeline Execution ↗ for commit b460601
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
@forgerock/davinci-client
@forgerock/device-client
@forgerock/journey-client
@forgerock/oidc-client
@forgerock/protect
@forgerock/recognize
@forgerock/sdk-types
@forgerock/sdk-utilities
@forgerock/iframe-manager
@forgerock/sdk-logger
@forgerock/sdk-oidc
@forgerock/sdk-request-middleware
@forgerock/storage
commit: |
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (14.28%) is below the target coverage (40.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #744 +/- ##
===========================================
+ Coverage 18.07% 71.22% +53.14%
===========================================
Files 155 60 -95
Lines 24398 5223 -19175
Branches 1203 965 -238
===========================================
- Hits 4410 3720 -690
+ Misses 19988 1503 -18485
🚀 New features to boost your workflow:
|
|
Deployed 9da7bd1 to https://ForgeRock.github.io/ping-javascript-sdk/pr-744/9da7bd189fd2e57669086dd681247320bb808152 branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🆕 New Packages🆕 @forgerock/journey-client - 92.6 KB (new) 📊 Minor Changes📈 @forgerock/oidc-client - 35.4 KB (+0.0 KB) ➖ No Changes➖ @forgerock/sdk-utilities - 18.6 KB 15 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📝 WalkthroughWalkthroughChangesThe package configuration declares CommonJS mode and adds the Public package exports
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/davinci-client/src/index.ts (1)
8-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd package-level tests for all new client exports.
The new public re-exports are uncovered, causing the patch to fail the 40% Codecov target.
packages/davinci-client/src/index.ts#L8-L9: test root imports fordavinciandfido.packages/device-client/src/index.ts#L7: test the rootdeviceClientexport.packages/oidc-client/src/index.ts#L7: test the rootoidcexport.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/davinci-client/src/index.ts` around lines 8 - 9, Add package-level tests covering the root exports: import and exercise davinci and fido from packages/davinci-client/src/index.ts (lines 8-9), deviceClient from packages/device-client/src/index.ts (line 7), and oidc from packages/oidc-client/src/index.ts (line 7). Follow existing package test conventions and ensure each public export is verified sufficiently to meet the Codecov threshold.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/davinci-client/src/lib/fido/index.ts`:
- Around line 8-10: Add focused coverage for the FIDO barrel in index.ts by
importing `@forgerock/davinci-client/fido`, exercising the exported fido path, and
adding a compile-time import that validates the re-exported types from
fido.types.js.
---
Nitpick comments:
In `@packages/davinci-client/src/index.ts`:
- Around line 8-9: Add package-level tests covering the root exports: import and
exercise davinci and fido from packages/davinci-client/src/index.ts (lines 8-9),
deviceClient from packages/device-client/src/index.ts (line 7), and oidc from
packages/oidc-client/src/index.ts (line 7). Follow existing package test
conventions and ensure each public export is verified sufficiently to meet the
Codecov threshold.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: de0f2a3e-7a90-483f-b255-4b5d444eccf0
📒 Files selected for processing (8)
package.jsonpackages/davinci-client/api-report/davinci-client.fido.api.mdpackages/davinci-client/package.jsonpackages/davinci-client/src/index.tspackages/davinci-client/src/lib/fido/fido.types.tspackages/davinci-client/src/lib/fido/index.tspackages/device-client/src/index.tspackages/oidc-client/src/index.ts
| // Re-export FIDO module and types for package.json `./fido` entry point | ||
| export { fido } from './fido.js'; | ||
| export * from './fido.types.js'; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Cover the new FIDO barrel before merging.
Codecov reports this changed file as uncovered, leaving patch coverage at 14.28571% against the 40% gate. Add a focused test that imports @forgerock/davinci-client/fido and exercises the exported fido path, with a compile-time import for the type re-exports.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/davinci-client/src/lib/fido/index.ts` around lines 8 - 10, Add
focused coverage for the FIDO barrel in index.ts by importing
`@forgerock/davinci-client/fido`, exercising the exported fido path, and adding a
compile-time import that validates the re-exported types from fido.types.js.
|
beta of this branch has been published to npm with tag |
1c03773 to
294f32a
Compare
vatsalparikh
left a comment
vatsalparikh
left a comment
There was a problem hiding this comment.
Comments on removing exports from associated types.ts file and the need for a separate fido/index.ts file
| @@ -1,4 +1,5 @@ | |||
| { | |||
| "type": "commonjs", | |||
There was a problem hiding this comment.
@ryanbas21 I changed it module but had to remove the requires from commitlint.config.js. Please review.
There was a problem hiding this comment.
ah that is my bad, this was the root package.json. It's fine as is, but it was a bad comment on my end, as it didn't need to be changed. It's fine as long as everythings working, i'd rather import/export personally.
294f32a to
7178144
Compare
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
At least one additional CI pipeline execution has run since the conclusion below was written and it may no longer be applicable.
Nx Cloud has identified a possible root cause for your failed CI:
We classified this failure as an environment issue rather than a code regression. All 4 tests time out at the very first UI interaction waiting for DaVinci flow buttons, and the cleanup phase also times out deleting the test user — both pointing to the external PingOne API being unreachable during this CI run. The PR only modifies export structure with no changes to business logic or API communication, so a rerun should resolve this.
No code changes were suggested for this issue.
Trigger a rerun:
🎓 Learn more about Self-Healing CI on nx.dev
|
Tests are failing on ValidatedPasswordCollector but I believe should be resolved once #757 is merged in. This migrates the DaVinci flow to the new tenant, improving both the flow and test. |
7178144 to
b460601
Compare
5 participants
JIRA Ticket
None
Description
This PR adds a new
./fidosubpath entry point to@forgerock/davinci-clientand ensures thedavinci,oidc, anddeviceClientfactories are explicitly exported from each package's default entry point.Why
DavinciClienttype depends onReturnType<typeof davinci>, it expectsdavincito be exported from types.Beta
beta of this branch has been published to npm with tag
0.0.0-beta-20260730211401